Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

refactor(platform): Add SVGs to projectTabs #673

Merged
merged 4 commits into from
Jan 30, 2025
Merged

refactor(platform): Add SVGs to projectTabs #673

merged 4 commits into from
Jan 30, 2025

Conversation

mrswastik-robot
Copy link
Contributor

@mrswastik-robot mrswastik-robot commented Jan 28, 2025 •
edited by codiumai-pr-agent-free bot
Loading

User description

Description

This code tries to add SVGs in the tab line for the projectTabs elements - Secret, Variable, Enivornment by modifying the line-tab.tsx file

Fixes #658

Screenshots of relevant screens

This is how it looks like right now, do let me know if further modifications are required ...

image

Developer's checklist

  • My PR follows the style guidelines of this project
  • I have performed a self-check on my work

If changes are made in the code:

  • I have followed the coding guidelines
  • My changes in code generate no new warnings
  • My changes are breaking another fix/feature of the project
  • I have added test cases to show that my feature works
  • I have added relevant screenshots in my PR
  • There are no UI/UX issues

Documentation Update

  • This PR requires an update to the documentation at docs.keyshade.xyz
  • I have made the necessary updates to the documentation, or no documentation changes are required.

PR Type

Enhancement

Description

  • Added SVG icons to Secrets, Variables, and Environment tabs.

  • Updated line-tab.tsx to support tab icons dynamically.

  • Introduced getIcon function to map tabs to respective SVGs.

  • Enhanced tab UI with icon and text alignment improvements.

Changes walkthrough 📝

Relevant files
Enhancement
line-tab.tsx
Add SVG icons and dynamic icon mapping to tabs                     

apps/platform/src/components/ui/line-tab.tsx

  • Imported EnvironmentSVG, SecretSVG, and VariableSVG components.
  • Added icon prop to Tab component for dynamic SVG rendering.
  • Implemented getIcon function to map tab names to SVG icons.
  • Adjusted tab layout to include icons alongside text.
    		• +26/-3   
    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
    •

    @codiumai-pr-agent-free CodiumAI PR-Agent (Free)
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis 🔶

    658 - Partially compliant

    Compliant requirements:

    • Add SecretSVG component to Secrets tab
    • Create and add VariableSVG component based on Figma design

    Non-compliant requirements:

    • Update tabs in project layout file (changes were made to line-tab.tsx instead)

    Requires further human verification:

    • Match desired UI design shown in screenshot - visual verification needed to ensure icons match Figma design exactly
    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Code Duplication

    The getIcon function could be moved outside the LineTab component since it doesn't use any component state or props, to avoid recreating it on every render

    const getIcon = (tab: string): React.ReactNode => {
  if (pathname.split('/')[1] !== 'project') return null

  switch (tab.toLowerCase()) {
    case 'secret':
      return <SecretSVG />
    case 'variable':
      return <VariableSVG />
    case 'environment':
      return <EnvironmentSVG />
    default:
      return null
  }
}

    @codiumai-pr-agent-free CodiumAI PR-Agent (Free)
    Copy link
    Contributor

    codiumai-pr-agent-free bot commented Jan 28, 2025 •
    edited
    Loading

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add icon rendering safety checks

    Add error boundary or null check for icon rendering to prevent potential runtime
    errors if SVG components fail to load or render.

    apps/platform/src/components/ui/line-tab.tsx [52-54]

     <span className="relative z-10 flex items-center gap-2">
-  {icon && <span className={'h-4 w-4'}>{icon}</span>}
+  {icon && (
+    <span className={'h-4 w-4'}>
+      {React.isValidElement(icon) ? icon : null}
+    </span>
+  )}
   {text}
 </span>
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion adds important runtime safety checks for icon rendering, preventing potential crashes if invalid icon elements are passed. This is a meaningful defensive programming practice.

    		7
    General
    Optimize icon function performance

    Memoize the getIcon function to prevent unnecessary recalculations on each render
    since it depends only on pathname and tab parameters.

    apps/platform/src/components/ui/line-tab.tsx [80-93]

    -const getIcon = (tab: string): React.ReactNode => {
+const getIcon = useCallback((tab: string): React.ReactNode => {
   if (pathname.split('/')[1] !== 'project') return null
   switch (tab.toLowerCase()) {
     case 'secret':
       return <SecretSVG />
     case 'variable':
       return <VariableSVG />
     case 'environment':
       return <EnvironmentSVG />
     default:
       return null
   }
-}
+}, [pathname])
    • Apply this suggestion
    Suggestion importance[1-10]: 5

    Why: Using useCallback for the getIcon function is a valid optimization that prevents unnecessary function recreations, though the performance impact would be minimal since the function is relatively simple.

    		5

    @kriptonian1
    Copy link
    Contributor

    @mrswastik-robot can you figure out a way in which the line highlighting the current tab, matches the width of the icon + the text, let me know if my explanation was not clear 😅

    @mrswastik-robot
    Copy link
    Contributor Author

    @mrswastik-robot can you figure out a way in which the line highlighting the current tab, matches the width of the icon + the text, let me know if my explanation was not clear 😅

    So it needs to be dynamic then ig? Changing according to the respective text + icon. Seems tough but I can try

    rajdip-b
    rajdip-b requested changes Jan 29, 2025
    View reviewed changes
    apps/platform/src/components/ui/line-tab.tsx Outdated Show resolved Hide resolved
    apps/platform/src/components/ui/line-tab.tsx Outdated Show resolved Hide resolved
    @mrswastik-robot
    Copy link
    Contributor Author

    Tried to made changes as requested by @rajdip-b , it was after the commit I saw that it was already resolved, should I revert back or it is fine as it is rn?

    @rajdip-b
    Copy link
    Member

    All good mate!

    @rajdip-b rajdip-b merged commit 37bfddf into keyshade-xyz:develop Jan 30, 2025
    4 checks passed
    rajdip-b pushed a commit that referenced this pull request Jan 30, 2025
    @semantic-release-bot
    chore(release): 2.11.0-stage.2 [skip ci]
    f9d05de 
    ## [2.11.0-stage.2](v2.11.0-stage.1...v2.11.0-stage.2) (2025-01-30)

### 🚀 Features

* **platform:** Add SVGs to projectTabs ([#673](#673)) ([37bfddf](37bfddf))
    @rajdip-b
    Copy link
    Member

    🎉 This PR is included in version 2.11.0-stage.2 🎉

    The release is available on GitHub release

    Your semantic-release bot 📦🚀

    @mrswastik-robot mrswastik-robot deleted the SVGs branch January 30, 2025 07:47
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Reviewers

    @rajdip-b rajdip-b rajdip-b approved these changes

    @kriptonian1 kriptonian1 Awaiting requested review from kriptonian1 kriptonian1 is a code owner

    Assignees
    No one assigned
    Labels
    released on @develop Review effort [1-5]: 2
    Projects
    None yet
    Milestone
    No milestone
    Development

    Successfully merging this pull request may close these issues.

    PLATFORM: Add icons to Secrets and Variables tab
    3 participants
    @mrswastik-robot @kriptonian1 @rajdip-b